Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exercises(list-ops): implement #557

Closed
wants to merge 24 commits into from
Closed

exercises(list-ops): implement #557

wants to merge 24 commits into from

Conversation

Norbiox
Copy link

@Norbiox Norbiox commented Jan 27, 2024

I have added missing exercise "list-ops" according to my proposition in this thread on forum.
Unfortunately I have failed to provide 2 tests and solutions for them - it's about handling nested lists in concatenate and reverse. I've tried generics but found no working solutions. I also looked at implementations of this task in other statically typed languages, but they've been missing those cases too. As I'm pretty new to statically typed languages I need to deepen my understanding of that topic.

Copy link

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jan 27, 2024
@ee7 ee7 reopened this Jan 27, 2024
@ee7 ee7 self-requested a review January 27, 2024 11:32
ee7 added 19 commits January 27, 2024 14:25
The `append`, `foldl`, and `foldr` procs each use `len`, so there's no
real benefit to pretending it doesn't exist in the `length` proc.

An alternative would be for this exercise to use some `distinct seq`,
but that's probably not worthwhile.
@ee7
Copy link
Member

ee7 commented Jan 27, 2024

Thanks. I took the liberty of pushing a first set of changes to your branch.

This is not yet ready to merge. I still want to at least:

  • Review the implemented test cases
  • Consider making append append in place
  • Consider avoiding built-in functions in the example, replacing add with append, and len with length

@ee7 ee7 changed the title Add exercise "list-ops" exercises(list-ops): implement Jan 27, 2024
@ee7 ee7 self-assigned this Jan 27, 2024
Copy link

@siebenschlaefer siebenschlaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests.toml

tests.toml should describe which tests from the canonical-data.json are implemented in the language-specific translation.
Lines 52-62 and 76-86 claim that three tests are not implemented but actually they are (lines 56-66 and 82-92).

test_list_ops.nim

The suite for foldl implements six tests (lines 55-78).
But the three latter tests are meant as replacements for the three former ones, specified via "reimplements"
(e.g. https://github.com/exercism/problem-specifications/blob/main/exercises/list-ops/canonical-data.json#L206).)
I'd suggest deleting the three tests in lines 56-66.

It's similar for the suite for foldr (lines 81-104), the latter three tests are meant as replacements for the former three tests.

The tests "empty list" in the suites for foldl (lines 68-70) and foldr (lines 94-96) define a function that adds its two parameters.
Why not stay true to the canonical-data.json and multiply the parameters?

The tests "direction dependent function applied to non-empty list" for foldl (lines 76-78 ) and foldr (lines 102-104) define a function that uses a subtraction.
The canonical-data.json uses a division and explicitly mentions in a comment that "if integer division is expected / required" the test data from the old version should be used.

The tests uses parameter names acc and el for the function that make it clear which argument is the accumulator.
see https://github.com/exercism/problem-specifications/blob/main/exercises/list-ops/canonical-data.json#L162-L165
They even reimplemented the three tests for foldl so this seems to be important.

The tests uses parameter names acc and el for the function that make it clear which argument is the accumulator.
see https://github.com/exercism/problem-specifications/blob/main/exercises/list-ops/canonical-data.json#L260-L263
They even reimplemented the three tests for foldl so this seems to be important.
I'd suggest using similarly expressive names instead of x and y.


list_ops.nim (stub)

In the initial stub the second parameter for foldl and foldr is defined as function: proc(x, y: int): int.
x and y are pretty generic names. IIRC that has confused some students and caused debates.
It was decided to rename these parameters to acc and el so that it would be obvious which one is the accumulator.
That was the reason why the three original tests for foldl and foldr were replaced.
I'd suggest using similarly expressive names instead of x and y, to stay close to the canonical-data.json and to avoid confusion.

The [canonical-data.json] contains a comment regarding the order of the arguments for the function that gets passed to foldl and foldr.
It basically says that there are different conventions, we should use the one common to the programming language.
Nim has the template foldl and foldr in the sequtils module.
foldl takes an expression where a is the accumulator and b the element,
whereas foldr takes an expression where a is the element and b is the accumulator.

I'd suggest changing the function in the "direction dependent" tests for foldr so that our foldr behaves similarly to sequtils.foldr:

do_assert sequtils.foldl(@[5, 7], a - b, 0) == -12
do_assert sequtils.foldl(@[1, 20, 300], a - b, 0) == -321
do_assert foldl(@[5, 7], func(acc, el: int): int = acc - el, 0) == -12
do_assert foldl(@[1, 20, 300], func(acc, el: int): int = acc - el, 0) == -321

do_assert sequtils.foldr(@[5, 7, 0], a - b) == -2
do_assert sequtils.foldr(@[1, 20, 300, 0], a - b) == 281
do_assert foldr(@[5, 7], func(el, acc: int): int = el - acc, 0) == -2
do_assert foldr(@[1, 20, 300], func(el, acc: int): int = el - acc, 0) == 281

example.nim

The example solution uses openArray almost everywhere for the parameters, whereas the solution stub uses seq.
Did you do that on purpose? How about openArray in the solution stub?

@Norbiox
Copy link
Author

Norbiox commented Jan 29, 2024

I should've read the canonical-data.json first I see. Somehow I missed it in introduction video.

To example.nim: I've used seq everywhere, but it was refactored to the point that It's not my PR anymore.

@Norbiox Norbiox closed this Jan 29, 2024
@Norbiox Norbiox deleted the exercise_list_ops branch January 29, 2024 06:21
@siebenschlaefer
Copy link

Oh, I'm sorry to hear that.

@Norbiox
Copy link
Author

Norbiox commented Jan 29, 2024

Me too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants